-
Notifications
You must be signed in to change notification settings - Fork 9
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support React 18 #423
Support React 18 #423
Conversation
Seems like we need to fix all of the workflows |
16e7e75
to
f8468e8
Compare
Legal Risk non-standard |
f8468e8
to
659ae41
Compare
Legal Risk non-standard |
659ae41
to
c97edf6
Compare
Legal Risk non-standard |
c97edf6
to
7054812
Compare
Legal Risk non-standard |
7054812
to
40e60b1
Compare
Legal Risk non-standard |
40e60b1
to
1d944ee
Compare
Legal Risk non-standard |
1d944ee
to
2360be9
Compare
Legal Risk non-standard |
2360be9
to
a157a75
Compare
Legal Risk non-standard |
Legal Risk non-standard |
Legal Risk non-standard |
Legal Risk non-standard |
Legal Risk non-standard |
J=BACK-2923 TEST=auto
5c087cc
to
d425b03
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the PR should point to develop rather than main
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like we use ReactDOM.render in the MapboxMap. we'll need to update that use the local react version's rendering approach
The following changes were made to support React 18:
react
andreact-dom
dev dependency versions from 17.0.2 to 18.2.0react-collapsed
from v3.6.0 to v4.1.2 which supports React 18@reach/auto-id
'suseId
hook with minor modifications from https://github.com/reach/reach-ui/blob/dev/packages/auto-id/src/reach-auto-id.ts. Because the peer dependencies of that library don't allow React 18, we weren't able to install it directly anymore. ThisuseId
hook uses React 18'suseId
hook if it's present and otherwise falls back to custom logic for React 16 and 17.renderPin
prop toMapboxMap
to allows React 18 users to render custom JSX into the DOM node for the marker element. This gets around the issue of having to callcreateRoot
fromreact-dom/client
inMapboxMap
. This was causing issues on React 16/17 sites because Vite and Webpack were unable to determine that the dynamic import ofreact-dom/client
wouldn't be reachable, and tried to resolve the package unsuccessfully. Rather than force existing consumers of the repo to update their bundler config to excludereact-dom/client
, we provide this optional prop to React 18 users if they want to use React 18 features in their custom pin component, or if they don't want to get the console warning about usingReactDOM.render
if they use thePinComponent
prop.To support testing in React 18:
@testing-library/react-hooks
doesn't support React 18 and it's functionality has now been added to@testing-library/react
in v14. Because of this,@testing-library/react
was upgraded to v14.2.1, which only supports React 18, and@testing-library/react-hooks
was removed from the dev dependencies. The GitHub workflow for testing React 16 and 17 was updated to manually downgrade@testing-library/react
to v12 and install@testing-library/react-hooks
. Custom logic is added so that if@testing-library/react-hooks
is installed, it'srenderHook
method is used (i.e. React 16/17), and otherwise therenderHook
method from@testing-library/react
is used (React 18).J=BACK-2911, BACK-2923
TEST=auto, manual
Updated the test-site to use the new React 18 app initialization and tested all updated components manually to ensure they're not broken. Did a local npm pack of the repo and successfully used it in Pages repos with React 17 and 18.
Some updates were also made to clean up our tests:
@testing-library/user-event
from v13.5.0 to v14.5.2, which makes async calls, so we no longer needwaitFor
. As part of this change, we started getting some new console errors when trying to navigate in tests by clicking links. This is because navigation is not defined in the jest jsdom environment. A util function was added to mock the console to remove these errors.